-
Notifications
You must be signed in to change notification settings - Fork 2
Update group to use Any #71
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good!
Left a few minor comments, some of them related to stuff that wasn't even affected by this PR but that I did notice in reviewing the code.
The main functional thing is to keep the DecisionPolicy
interface as generic as possible and not add things that are just specific to ThresholdDecisionPolicy
.
incubator/group/codec.go
Outdated
// | ||
// The actual codec used for serialization should be provided to x/transfer and | ||
// defined at the application level. | ||
ModuleCdc = codec.NewHybridCodec(amino, cdctypes.NewInterfaceRegistry()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to make this private?
incubator/group/group.go
Outdated
} | ||
return data.Validate() | ||
} | ||
|
||
func (a AppModule) RegisterRESTRoutes(ctx context.CLIContext, r *mux.Router) { | ||
func (a AppModuleBasic) RegisterRESTRoutes(ctx context.CLIContext, r *mux.Router) { | ||
//rest.RegisterRoutes(ctx, r, ModuleCdc, RouterKey) | ||
// todo: what client functions do we want to support? | ||
panic("implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to panic here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's existing code from @alpe I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know, just noting it as something to cleanup
@@ -2,50 +2,64 @@ package testdata | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish there were a way we could just extend simapp
from the sdk rather than just having everything copied here. Not relevant for this PR, but something to think about in the future.
Co-authored-by: Aaron Craelius <aaron@regen.network>
incubator/group/group.go
Outdated
} | ||
|
||
func (a AppModuleBasic) GetQueryCmd(*codec.Codec) *cobra.Command { | ||
//return cli.GetQueryCmd(StoreKey, cdc) | ||
panic("implement me") | ||
return &cobra.Command{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Allow(tally Tally, totalPower sdk.Dec, votingDuration time.Duration) (DecisionPolicyResult, error) | ||
Validate(g GroupMetadata) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a godoc here explaining why GroupMetadata
is passed in?
closes: #69